Skip to content

Do install msys2 in CI #136810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Feb 10, 2025

Reverts #136588.
Only papers over #136795, but the root cause would still need to be investigated (likely a bug in how i686-pc-windows-gnu is built?).

Seems like for whatever reason the msys2 installation here is needed for the 32-bit i686-pc-windows-gnu target, otherwise we seem to be shipping the wrong DLL. This revert is mostly just to buy time to investigate without having to debug why the build logic for the 32-bit windows-gnu target is wrong.

This reverts commit e060723, reversing changes made to a9e7b30.

r? infra
cc @ChrisDenton @Kobzol @mati865

try-job: dist-x86_64-msvc
try-job: dist-i686-msvc
try-job: dist-aarch64-msvc
try-job: dist-i686-mingw
try-job: dist-x86_64-mingw
try-job: dist-x86_64-msvc-alt

Seems like for whatever reason the msys2 installation here is needed for
the 32-bit `i686-pc-windows-gnu`, otherwise `rustc` distributed for
that target seems to link against the 64-bit libraries or sth. This
revert is mostly just to buy time to investigate.

This reverts commit e060723, reversing
changes made to a9e7b30.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2025
@jieyouxu
Copy link
Member Author

@bors try

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-windows-gnu Toolchain: GNU, Operating system: Windows T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2025
Do install msys2 in CI explicitly

Reverts rust-lang#136588.
Only papers over rust-lang#136795, but the root cause needs to be investigated (likely a bug in how `i686-pc-windows-gnu` is built?).

Seems like for whatever reason the msys2 installation here is needed for the 32-bit `i686-pc-windows-gnu` target, otherwise `rustc` distributed for that target seems to link against the 64-bit `libwinpthread-1.dll` and such. This revert is mostly just to buy time to investigate without having to debug why the build logic for the 32-bit windows-gnu target is wrong.

This reverts commit e060723, reversing changes made to a9e7b30.

r? infra
cc `@ChrisDenton` `@mati865`

try-job: dist-x86_64-msvc
try-job: dist-i686-msvc
try-job: dist-aarch64-msvc
try-job: dist-i686-mingw
try-job: dist-x86_64-mingw
try-job: dist-x86_64-msvc-alt
@bors
Copy link
Collaborator

bors commented Feb 10, 2025

⌛ Trying commit e35890a with merge 1dda29f...

@jieyouxu jieyouxu force-pushed the revert-msys2-removal branch from e35890a to f55585e Compare February 10, 2025 10:05
@jieyouxu jieyouxu changed the title Do install msys2 in CI explicitly Do install msys2 in CI Feb 10, 2025
@ChrisDenton ChrisDenton reopened this Feb 10, 2025
@ChrisDenton
Copy link
Member

Oops, sorry wrong button

@jieyouxu
Copy link
Member Author

Inspected the dist toolchain locally with

rustup-toolchain-install-master 1dda29fe16a5e22031ce4c5080c17dcc2ac04ac2 --host=i686-pc-windows-gnu --targets=i686-pc-windows-gnu

I believe reverting somehow makes us ship the right libwinpthread-1.dll:

C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools> dumpbin /HEADERS C:\Users\Administrator\.rustup\toolchains\1dda29fe16a5e22031ce4c5080c17dcc2ac04ac2\lib\rustlib\i686-pc-windows-gnu\bin\libwinpthread-1.dll
Microsoft (R) COFF/PE Dumper Version 14.43.34604.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file C:\Users\Administrator\.rustup\toolchains\1dda29fe16a5e22031ce4c5080c17dcc2ac04ac2\lib\rustlib\i686-pc-windows-gnu\bin\libwinpthread-1.dll

PE signature found

File Type: DLL

FILE HEADER VALUES
             14C machine (x86)
[... omitted ...]

OPTIONAL HEADER VALUES
             10B magic # (PE32)
            2.39 linker version
[... omitted ...]

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 10, 2025

This is technically ready for review, but I wouldn't be opposed to close this in favor of somehow fixing the PATH issue as discussed in https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/Changes.20to.20i686-pc-windows-gnu or e.g. @ChrisDenton's attempts over at #136812.

This PR is only opened as a safety net.
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2025
@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 10, 2025

I may have a fix in #136815 but it still needs to be tested.

EDIT: tested, and it works!

@jieyouxu
Copy link
Member Author

Since #136815 seems to work, closing in favor of that.

@jieyouxu jieyouxu closed this Feb 10, 2025
@jieyouxu jieyouxu deleted the revert-msys2-removal branch February 10, 2025 15:33
@workingjubilee workingjubilee added the O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) label Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows-gnu Toolchain: GNU, Operating system: Windows O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants